Add Django 2.x to testing matrix. #1
Conversation
Fix up settings and skip certain tests that we probably don't care about.
| @@ -68,8 +70,10 @@ def test_post_process(self): | |||
| self.assertTrue(('screen.css', 'screen.css', True) in processed_files) | |||
| self.assertTrue(('scripts.js', 'scripts.js', True) in processed_files) | |||
|
|
|||
| @unittest.skip("don't care") | |||
There was a problem hiding this comment.
Doesn't this test a condition we'll soon have, with the s3 boto storage (which doesn't have a path method implemented). Was this disabled because it's failing, or for some other reason?
| @@ -249,7 +249,7 @@ def test_cssmin(self): | |||
| self._test_compressor('pipeline.compressors.cssmin.CSSMinCompressor', | |||
| 'css', 'pipeline/compressors/cssmin.css') | |||
|
|
|||
| @skipUnless(settings.HAS_NODE, "requires node") | |||
| @skip("don't care") | |||
There was a problem hiding this comment.
We have this compressor re-implemented and modified in our code base, so if it's failing, we ought to make sure that our rewrite isn't affected by the same issue.
There was a problem hiding this comment.
All of the failures that I've skipped fail in all Django versions tested (1.11-2.2). I'm not aware if -- or when -- we've ever run this test suite, and as much as I agree with your concerns, I think we should add a similar test as this and the others in Ollie, so that we can catch and fix any pertinent failures.
There was a problem hiding this comment.
Or, if it's passing in travis but failing locally, I can probably help with the configuration. I actually wrote these tests; they were merged into upstream a while back (jazzband#531)
There was a problem hiding this comment.
It might just be a better-documentation thing that I can flesh out (meaning, the local tox failures), so if you have any suggestions beyond what I added to the README, I can give that a try.
There was a problem hiding this comment.
Also, as for the Java tests that I skipped, I was assuming that we didn't need any of those. If that is incorrect, I guess I can install the Java runtime, but I'd really hate to go down that rabbit hole locally.
There was a problem hiding this comment.
I believe that you need to run npm install from the repo root for some of these tests to pass. Beyond that, you might need to rebase jazzband/django-pipeline@3a0e3cc and jazzband#644. If those don't do the trick, then I think it's not worth the trouble—in which case this PR LGTM
There was a problem hiding this comment.
Just saw your comment now about the java tests: we don't need these. They could perhaps be wrapped in a skipUnless for settings.HAS_JAVA, but that's a very nitpicky concern and I think it's also fine as is.
There was a problem hiding this comment.
I definitely did npm install. Unless I was too quick about it, when I stepped through this particular test failure, it seemed to me that the custom test storage class PipelineNoPathStorage was the cause of the error.
There was a problem hiding this comment.
On further reflection, I think the sourcemap changes on this branch would need to be rebased with the commits from jazzband#490, which is a mostly-abandoned upstream PR that might differ from the sourcemap implementation on atl/1.6.x in subtle ways. So I have no more objections to disabling the tests in this PR.
Fix up settings and skip certain tests that we probably don't care about.